Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changed SuperCell to Lattice, long overdue #550

Merged
merged 2 commits into from
Mar 15, 2023
Merged

changed SuperCell to Lattice, long overdue #550

merged 2 commits into from
Mar 15, 2023

Conversation

zerothi
Copy link
Owner

@zerothi zerothi commented Mar 14, 2023

The ambiguity of referencing to the lattice/unit-cell as a supercell is confusing when we are also dealing with the description of the number of supercells.
Therefore we have changed all SuperCell references and sc arguments to Lattice and lattice, respectively.

This surely comes at a cost for end-users, since this is a major transition. I will keep backwards compatibility with the older implementations for at least a year (perhaps 2). But they should eventually be gone to prefer the lattice.

@tfrederiksen @pfebrer please chip in here, the discussion about this entered in #95. I have long wanted to change this since it is a bit ambiguous in its meaning.
This change will allow it to differ from "supercells" which I think is much clearer.
The Lattice name was basically chosen to continue the .cell access of the lattice vectors.
I have thought about this for some time, but if you have suggestions, please do let me know!
The initial discussion also considered:

  • class: Cell, lattice vectors abc
  • class: Cell, lattice vectors cell (a little problematic IMHO)
  • Closes sc arguments should be named cell #95
  • Tests added (I think I covered everything)
  • Documentation for functionality
  • User visible changes (including notable bug fixes) are documented in CHANGELOG

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@pfebrer
Copy link
Contributor

pfebrer commented Mar 14, 2023

My first thought is that lattice is nicer than supercell, it feels more natural 👍

Then there could also be a Sites class and a Geometry would be the combination of Lattice, Sites and Atoms. A combination of only Sites and Lattice could implement the purely geometrical methods that are now implemented in Geometry, which maybe would be useful for other things. The first thing that comes to mind is k points in the reciprocal lattice, but I'm sure it could have other applications e.g. in situations where you have a sparse sampling of space :)

The ambiguity of referencing to the lattice/unit-cell
as a supercell is confusing when we are also dealing with
the description of the *number of supercells*.
Therefore we have changed all SuperCell references and sc
arguments to Lattice and lattice, respectively.

This surely comes at a cost for end-users, since this is
a major transition. I will keep backwards compatibility
with the older implementations for at least a year (perhaps
2). But they should eventually be gone to prefer the lattice.

Signed-off-by: Nick Papior <[email protected]>
@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #550 (2d45aad) into main (1a08992) will increase coverage by 0.03%.
The diff coverage is 94.30%.

@@            Coverage Diff             @@
##             main     #550      +/-   ##
==========================================
+ Coverage   86.20%   86.23%   +0.03%     
==========================================
  Files         370      370              
  Lines       46702    46777      +75     
==========================================
+ Hits        40258    40338      +80     
+ Misses       6444     6439       -5     
Impacted Files Coverage Δ
sisl/_typing.py 0.00% <ø> (ø)
sisl/typing/_common.py 0.00% <ø> (ø)
sisl/io/openmx/omx.py 16.11% <44.44%> (ø)
sisl/io/bigdft/ascii.py 78.26% <60.00%> (ø)
sisl/physics/brillouinzone.py 87.37% <66.66%> (ø)
sisl/physics/sparse.py 88.60% <69.23%> (ø)
sisl/io/siesta/fdf.py 78.03% <74.57%> (+0.70%) ⬆️
sisl/io/scaleup/ref.py 75.75% <75.00%> (ø)
sisl/physics/densitymatrix.py 90.85% <75.00%> (-0.02%) ⬇️
sisl/physics/tests/test_brillouinzone.py 92.71% <78.57%> (ø)
... and 77 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tfrederiksen
Copy link
Contributor

I like Lattice too, as well as @pfebrer's suggestion of Sites.

Note that in 3D there exists only 14 different Bravais lattices. Maybe they could be introduced for some symmetry-related functionality?

@zerothi
Copy link
Owner Author

zerothi commented Mar 15, 2023

Ok, so let's merge this and be gone with the madness. @tfrederiksen once merged, it would be nice to see if hubbard works with no changes, I have tried to catch tests as much as possible, but with this major change I have the feeling that I forgot something ;)

@zerothi zerothi merged commit 18bc2c1 into main Mar 15, 2023
@zerothi zerothi deleted the sc-to-lattice branch March 15, 2023 06:43
@zerothi zerothi mentioned this pull request Mar 15, 2023
@tfrederiksen
Copy link
Contributor

With inspiration from spglib, one could rename .cell to .basis (basis vectors) and reserve cell for information about the connectivity etc.

@zerothi
Copy link
Owner Author

zerothi commented Mar 15, 2023

Hmm, I am not too fond of that convention. Kind of like cell like unit-cell. I don't fully get your last paragraph?

@zerothi
Copy link
Owner Author

zerothi commented Mar 15, 2023

I was considering abc or cell, I think basis is a little more "mathy", and not so physics oriented. Could be wrong though?

@tfrederiksen
Copy link
Contributor

tfrederiksen commented Mar 15, 2023

I am just wondering if the notion of nsc is clear for the user, and if basis would be a more natural property of Lattice.

But I'm also fine with cell as it is.

@zerothi
Copy link
Owner Author

zerothi commented Mar 15, 2023

agreed, nsc could probably be spelled out supercells. I just hate long words... ;)

@tfrederiksen
Copy link
Contributor

Isn't it rather that a supercell means the collection of the unit cell + its neighbours?

@zerothi
Copy link
Owner Author

zerothi commented Mar 15, 2023

yes, but that is essentially what nsc means, no? Or should we do nsc = cell_neighbours, or something similar? The supercell phrase is just used extensively (in the code and in literature).

@zerothi zerothi mentioned this pull request Mar 15, 2023
@tfrederiksen
Copy link
Contributor

tfrederiksen commented Mar 15, 2023

Another word could be connections.

@zerothi
Copy link
Owner Author

zerothi commented Mar 15, 2023

That's not entirely fitting either, is it orbital connections, or ...

Hmm... in the sparse structures we use Sparse.edges but neither are referring to the clarity of supercells, I think. The fact that this is an auxiliary supercell I think is quite vital in the understanding of its use, and if that gets lots in the name, I am afraid it will confuse?

@pfebrer
Copy link
Contributor

pfebrer commented Mar 15, 2023

It is weird because nsc is not really a property of the lattice, no? It only makes sense in a combination of a lattice, some sites, and something that defines the range of the sites. Perhaps the lattice information should be separated from the auxiliary cell information, which could be an attribute -aux_cell- of Geometry or whatever other class where it makes sense.

So lattice would hold cell and pbc, and aux_cell would hold nsc.

@tfrederiksen
Copy link
Contributor

Supercells in plural is what I find a bit confusing terminology.

@zerothi
Copy link
Owner Author

zerothi commented Mar 15, 2023

It is weird because nsc is not really a property of the lattice, no? It only makes sense in a combination of a lattice, some sites, and something that defines the range of the sites. Perhaps the lattice information should be separated from the auxiliary cell information, which could be an attribute -aux_cell- of Geometry or whatever other class where it makes sense.

So lattice would hold cell and pbc, and aux_cell would hold nsc.

True... The plot thickens...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sc arguments should be named cell
3 participants